Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close connection to server on connect errors #647

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

luddd3
Copy link
Contributor

@luddd3 luddd3 commented Sep 18, 2021

A minimal pull request to fix open socket leak after e.g. authentication error on connect. This is a part of the fix in #584, but I have only kept the most essential part and added a test.

By keeping this as small as possible, I hope to remove any hesitance so that it can be merged.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 3, 2021

Never heard of the concept of bike shedding? :D

@luddd3
Copy link
Contributor Author

luddd3 commented Dec 28, 2021

@kibertoad Sorry for pinging, but I saw in another thread that you do some work in this repo. Can you please review this PR too?

@kibertoad kibertoad merged commit 70a49a4 into amqp-node:main Dec 28, 2021
@kibertoad
Copy link
Collaborator

Thank you!

@kibertoad
Copy link
Collaborator

@luddd3 CI is failing on main now, could you please take a look?

@luddd3
Copy link
Contributor Author

luddd3 commented Dec 28, 2021

@kibertoad Fixed in #667
I think there are many steps during setup that the openCallback might be called more than once. For example, what if the socket is abruptly closed in-between the steps? Then bail would be called multiple times.

@luddd3 luddd3 deleted the connect-error branch December 28, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants